Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow all mempool txs to be replaced after a configurable timeout (default 6h) #10823

Conversation

greenaddress
Copy link
Contributor

This PR' aim is to improve user experience around stuck transactions without
affecting users of zero conf transactions.

tldr: Allow transaction replacement for transactions sitting in mempool for
longer than timeout (default 6h configurable) regardless of opt-in replacement
flag.

This PR affects policy/relay only.

Stuck transactions have been a problem for users recently. While wallets
are improving (opt in replacement, Child Pays For Parent, etc) there are some
cases which find users with transactions stuck for days that can't be solved
easily/reliably by wallet developers, especially when the user creates the
stuck transaction with old software or for some reason disabled available
features countering stuck transactions.

For the purpose of the below I will ignore transactions created by the core
wallet when talking about transaction expiration/eviction and focus on policy.

Bitcoin 0.12 introduced (or in a way re-introduced) opt-in transaction
replacement (BIP125), allowing people to more explicitly flag that their
transaction can be replaced (such that users of zero conf transactions can
immediately recognize them).

At the same time mempool limiting (configurable) was introduced, making the
individual mempool drop transactions at the bottom (low fee) when full.

Both before and after these changes any transaction in mempool would be
automatically evicted after 72 hours (configurable).

Recently 0.14.0 increased the eviction from 72 hours to 2 weeks. These changes
allows users of the system to aim for lower fees but at the same time makes it
frustrating for users that disable opt-in transaction replacement or that use
software that doesn't support it in first place to bump the fee at a later time
or to revert the payment as they have to wait for a while or use ad-hoc
software.

A number of miners will mine transactions regardless of opt-in flags (5-10%
maybe) and while core nodes won't propagate those transactions, a well
connected user can generally get replacement transactions mined within a
reasonable amount of time without opt-in transaction replacement flags set.

This may be convenient for attackers or ad-hoc expert use but
not ideal for wallet developers, or at least until core merges full transaction
replacement because using this functionality would requires wallets to use
preferential peering and/or forks of bitcoin core.

Until then a compromise solution that doesn't impact zero conf use and that
improves user experience would be to allow transactions to be replaced after
sitting for a timeout in mempool (thus unconfirmed).

The timeout should be high enough that allows current use of zero conf and at
the same time allows same working day solution for users.
I suggest a 6 hours timeout and to have it configurable for testing and ability
for user to change.

The changes continue to support disabling entirely transaction replacement
(-mempoolreplacement) and introduces a new command line parameter
(-mempoolreplacementtimeout) which allows to pass the number of seconds after
which a transaction can be unconditionally replaced and setting this parameter
to two weeks will keep the original behavior.

If you want to test the changes using @petertodd Replace-by-Fee tools build
core with this PR applied and wallet enabled and run with
-mempoolreplacementtimeout=10 and use doublespend.py (with and without
-b 1) from https://github.com/petertodd/replace-by-fee-tools

@greenaddress greenaddress force-pushed the replace-by-fee-old-transactions branch from e166d4e to bea0461 Compare July 14, 2017 15:38
@jonasschnelli
Copy link
Contributor

I think such policy changes should first be discussed on the bitcoin-dev mailing list and eventually deserve a BIP.

src/validation.cpp Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Contributor

Yea, agreed that this should get a BIP (sadly probably means endlessly trolled), but does seem awesome to me. Does this need a new option? We dont currently have an option for opt-in-rbf, why not just leave this as hardcoded policy?

@greenaddress
Copy link
Contributor Author

greenaddress commented Jul 14, 2017

@TheBlueMatt replacement can be disabled (as a node policy afaik rather than wallet) and the new option i added allows for easier testing and for people to run values that they like different from the default (either to be same as previous behavior or to get the other end without running some fork/patched core).

@TheBlueMatt @jonasschnelli I am happy to do a BIP and discuss in the dev mailing list as needed. I thought the changes may be borderline like the change from 72h to 2 weeks for mempool expiry but happy (minus potential trolling) to learn otherwise and or try to reduce the impact of the changes if this is an option.

@petertodd
Copy link
Contributor

I disagree that this needs a BIP.

Opt-in RBF added a new way to interpret a transaction, which just barely qualified as something you might want to do a BIP for.

This however makes an existing behavior - transactions being replaceable in spite of them not signalling opt-in RBF - happen a little sooner in some circumstances, just like adding expiration did in the first place. We didn't create a BIP for expiry, so why does this need a BIP?

Secondly, writing a BIP for such a trivial change gives the misleading impression that you could rely on the opposite behavior. We've had continual problems with people misunderstanding the security properties of zeroconf; there's no reason to add to that problem.

@luke-jr
Copy link
Member

luke-jr commented Jul 15, 2017

Indeed, mere policies are not BIP material...

@gmaxwell
Copy link
Contributor

People might want to know about it... and a BIP might be a good way to communicate it... but we certainly didn't write a BIP about the expiration time changes over time, and this is strictly a narrower change.

Dunno how ideal 6 hours is though.

@EagleTM
Copy link

EagleTM commented Jul 16, 2017

I'd suggest to put the timeout at 72h - at least when introducing the functionality. This way the behviour is similar to pre 0.14.x code. It's less likely to create havoc / confusion for operators who are still used to regularly see 0-conf transactions being re-spent after that time-frame (but not earlier).

@petertodd
Copy link
Contributor

@gmaxwell I suggested six hours because it's more than long enough (36 blocks) that if you wanted that tx mined relatively quickly, you're probably getting annoyed that it isn't.

@EagleTM Try actually doublespending some time - it's a lot easier than you think it is.

@rubensayshi
Copy link
Contributor

I think 6hrs is very short, I've seen many TXs confirmed after 24hrs+, there's plenty of people who, during bigger mempool periods, even try to aim for that...

though it might feel a bit insignificant for a BIP, this PR impacts a lot of assumptions people have about 0conf txs, it would get more significant exposure and discussion if proposed as a BIP.

@petertodd
Copy link
Contributor

@rubensayshi You seem to be arguing that the time interval should be longer for security reasons. For that argument to be valid, you'd have to substantiate the claim that a transaction with a fee so low that it fails to confirm after 36 blocks - 10 blocks more than the fee estimator even supports - is still difficult to double spend.

@greenaddress
Copy link
Contributor Author

@rubensayshi this doesn't stop those transactions from being confirmed if that's what the user wanted anyway it just allows people to replace transactions after a 6 hours long period - replacing them is already very easy to do ad-hoc even without the replacement flag but that full replacement it is currently painful to do for wallet developers without having to use different peering policies/full nodes with full replacement enabled.

I'd argue for full replacement at all times given the farce that it is doing replacement even without flags but if people want to at least try to keep the illusion of zero conf then I think 6 hours gives plenty of time for people that use zero conf to keep using it at the same ~ [in]security as today inclusive of illusion while giving users the opportunity to not have txs stuck for 2 weeks and solve stuck transactions problems within a business day.

@greenaddress
Copy link
Contributor Author

greenaddress commented Aug 1, 2017

conflicts; should I rebase and squash while at it?

@petertodd
Copy link
Contributor

@greenaddress +1

@greenaddress greenaddress force-pushed the replace-by-fee-old-transactions branch from 38f4c85 to 8c223a3 Compare August 3, 2017 17:03
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, although it may be desirable to have some way to explicitly set the "opt-in only" policy.

Would also prefer -mempoolreplacement=fee,timeout=N syntax for the config

@greenaddress
Copy link
Contributor Author

@luke-jr not sure i understand your first point

I rebased as there was a conflict (new gArgs) and made sure feebumper.cpp handles the timeout too and added a test.

src/validation.cpp Outdated Show resolved Hide resolved
src/wallet/feebumper.cpp Outdated Show resolved Hide resolved
test/functional/bumpfee.py Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
@greenaddress greenaddress force-pushed the replace-by-fee-old-transactions branch 3 times, most recently from dd0bb01 to 9276665 Compare December 9, 2017 12:06
@greenaddress
Copy link
Contributor Author

greenaddress commented Dec 9, 2017

@MarcoFalke rebased

@promag moved to setmocktime, factored out the function, removed the white space and updated to new style - thanks!

More feedback welcome. I think it would be good to have more tests (ideas?) and I tried to be the least intrusive here but perhaps people have some refactoring suggestions.

edit: there's a bit more work needed as something broke with the rebase around list transactions/mempool sync

@petertodd
Copy link
Contributor

utACK 927666539bd61da5038b752ee60d106b9577ca42

test/functional/bumpfee.py Outdated Show resolved Hide resolved
test/functional/bumpfee.py Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
test/functional/bumpfee.py Outdated Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
@promag
Copy link
Member

promag commented Dec 10, 2017

This test is failing

assert_array_result(self.nodes[0].listtransactions(), {"txid": txid_4}, {"bip125-replaceable":"unknown"})

@greenaddress greenaddress force-pushed the replace-by-fee-old-transactions branch 2 times, most recently from 039dcfb to 4fc450a Compare December 13, 2017 12:46
@maflcko
Copy link
Member

maflcko commented Mar 4, 2019

Adding 0.19.0 milestone. If the 6h is too controversial even though it probably represents a sane default per @Sjors, it could be bumped higher to avoid the controversy.

@greenaddress greenaddress force-pushed the replace-by-fee-old-transactions branch from a220fca to 03fa5a1 Compare March 8, 2019 13:52
@greenaddress
Copy link
Contributor Author

@MarcoFalke rebased and addressed some formatting improvement suggestions on the test

@gmaxwell
Copy link
Contributor

Subsequently to my earlier comments I now think this is kinda pointless: Testing without RBF set gave me 100% confirmation or replacement rate for very low fee transactions within 20 blocks without the low fee txn rising to being minable by feerate presumably due to restarting nodes, and miners that replace anyways. Is this just motivated by either speculation on actual behaviour without measuring it or a distributed systems misunderstanding that causes people to not just set replacement in the first place?

@petertodd
Copy link
Contributor

Gotta agree with @gmaxwell here.

Also, the myth that zeroconf transactions have any security recently lead to $195,000 CAD of publicly reported losses: https://globalnews.ca/news/5047918/calgary-police-nationwide-bitcoin-fraud/

Full-RBF-by-default will help mitigate this misunderstanding. I also have personal interest in stopping this misunderstanding as other reporting on the subject has inaccurately implicated me as part of this crime.

@maflcko
Copy link
Member

maflcko commented Apr 5, 2019

I disagree.

It is highly unlikely that regular users will use bumpfee (or other use cases of tx replacements) merely because observations suggest that miners are accepting full-rbf. This pull request would make it also mempool policy, which is useful when a wallet is not directly connected to miners, but maybe needs to do a few hops through network nodes.

@DrahtBot
Copy link
Contributor

Needs rebase

@Sjors
Copy link
Member

Sjors commented Apr 23, 2019

I did an n=1 experiment today where, trying to replace a single-input non-RBF transaction that pays 15 sat / vbyte with one that pays 50 sat / vbyte.

The new transaction doesn't show up on any explorer, suggesting none of my peers is relaying it despite the huge fee bump. It's not like there's a list of known full-RBF peers to manually connect to either.

This is potentially different from the very low fee scenario Maxwell described, because my initial transaction was very well propagated.

The procedure is slightly easier than it used to be, because there's no need to zap the wallet. Just unload the wallet and delete mempool.dat before calling sendrawtransaction.

That said, I don't think my example is a good use case. The reason the initial transaction didn't use RBF is because it was created with RPC and I forgot to set walletrbf=1. I also just noticed walletcreatefundedpsbt default replaceable argument ignores this setting (#15878).

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces two changes:

  1. a policy change to allow mempool transactions to be replaced after a configurable time
  2. allow wallets to create replacement transactions after a configurable time

I think those changes should be separated, at least into two separate commits.

My preference for (2) would be for the behavior to be controlled by a flag in the bumpfee RPC to force replacement rather than a flag enablewalletreplacementtimeout which in your current implementation is shared between node/wallet.

I'm fine having the default set to 72 hours to address the concerns of @ziggamon and others.

I think it would be good to have more tests (ideas?)

The current test is for rejection/acceptance by the wallet. Could you add a test that tries to submit a replacement tx over P2P which is rejected, and then setmocktime in the future and have the tx accepted over P2P?

return now - accepted >= timeout;
}

RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool, const int64_t now, const int64_t timeout, const bool enabled_replacement_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of changing the semantics of this function. Previously it meant "does this transaction (or one of its unconfirmed parents) signal opt-in RBF?". Now it means "does my mempool configuration allow this tx to be replaced?"

That means that the bip125-replaceable field in getmempoolentry would now show true for transactions that aren't replaceable under bip125 rules for example.

src/validation.cpp Show resolved Hide resolved
static const int64_t DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;
/** Default for buffer in seconds on top of the timeout after which transactions in mempool are replaceable (i.e. 6 hours) in the GUI
* This buffer is needed because otherwise is more likely ours peers will reject the transaction in case they received it substantial time after us */
static const int64_t REPLACEMENT_TIMEOUT_BUFFER = 5 * 60;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this constant to feebumper.cpp since it's only used there.

@@ -516,6 +516,8 @@ void SetupServerArgs()
gArgs.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (default: %u)", DEFAULT_ENABLE_REPLACEMENT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacementtimeout=<n>", strprintf("Number of seconds after which transactions in mempool can be replaced (default: %u)", DEFAULT_REPLACEMENT_TIMEOUT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-enablewalletreplacementtimeout", strprintf("Whether to enable wallet replacement timeout (default: %u)", DEFAULT_WALLET_REPLACEMENT_TIMEOUT), true, OptionsCategory::OPTIONS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this option entirely and replace it with an argument to bumpfee

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this option entirely and add wallet and rpc support after the p2p change is merged

@ziggamon
Copy link

ziggamon commented May 8, 2019

Before going through the code, Is there any consensus on whether this is a good idea at all? Reminding of the debate around opt-in RBF. The opponents' argument was that it would lead to a slippery slope and removing 0-conf entirely. This seems like a step in this direction. Like it or not it is successfully used to a large extent, not just by our users.

Let's evaluate some situations:
a) In status quo (i.e. mempool clears out almost every night) there is no need for this.

b) In a state where the mempool is non-empty for long periods of time this creates a situation that potentially worsens a fee crisis by making it so that merchants must CPFP transactions to fit within the 72h (or whichever window), which potentially can lead to trampling for the exits at the exact wrong time, causing fees to spike further. Large amounts of automated code outbidding eachother for limited space is part of the reason for why we had the fee problem in 2017 in the first place.

CPFP here also becomes a vector for abuse per the following: Anyone can make a large consolidation transaction that includes a send to a party that does CPFP on incoming tx's within 72h => Externalize the costs of confirming this consolidation to someone else, which in this scenario is expensive.

  • Question here: Would a child transaction prevent this from running or would it run regardless? Or is there some other mechanism by which a recipient could disable this functionality?

Can somebody describe a specific scenario where a user would benefit from this change in network policy? Is this a user that uses an RBF wallet, but chooses to have RBF disabled but then changes his mind? Or is it a user that upgrades wallets in order to access this feature post-fact? Can we estimate the potential benefit and weigh against the potential damage?

I agree with John that we should separate policy aspects here from wallet aspects. Bitcoind policy for propagation is not consensus but largely goes unchanged so usually becomes a sort of de facto consensus.

@jnewbery
Copy link
Contributor

jnewbery commented May 8, 2019

Is there any consensus on whether this is a good idea at all?

No, I don't think consensus has been reached. The fact that you have legitimate concerns about it shows that we don't have consensus!

In status quo (i.e. mempool clears out almost every night) there is no need for this.

Correct - if the mempool clears out almost every night then there is no need for this.

Would a child transaction prevent this from running or would it run regardless? Or is there some other mechanism by which a recipient could disable this functionality?

With the code in this PR, no. Descendants are ignored when considering whether a transaction is eligible for replacement.

Can somebody describe a specific scenario where a user would benefit from this change in network policy? Is this a user that uses an RBF wallet, but chooses to have RBF disabled but then changes his mind?

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet). If a user of one of those wallets sends a transaction using the defaults, finds that it's stuck and later wishes to bump the fee, they are unable to. If the defaults for policy were changed according to this PR, that user would be able to bump their transaction after 6 hours (or 72 hours if we changed the default time).

My main argument for this change is that it makes expectations clearer. As @greenaddress, @petertodd and @gmaxwell have pointed out, if a transaction is unconfirmed after 6 hours, a malicious actor could easily have double-spent it. Any recipient of a transaction that hasn't been confirmed after that much time should therefore not have any confidence that the tx will ever confirm.

@SomberNight
Copy link
Contributor

Can somebody describe a specific scenario where a user would benefit from this change in network policy?

Near the end of 2017, "how much time do I need to wait until my transaction gets cancelled" was a frequent question on IRC. The usual scenario was that the user had used either old software, or just a shitty wallet, that did not set RBF, and had set a low fee. This tx would then never get mined. In some unlucky cases, there was not even a change output, so CPFP could not be used.

Not sure how much wallets improved since then. I guess we will only really know when the mempool gets consistently non-empty again.

This PR, being able to RBF transactions without opt-in after some time, would be useful so that at least users could be given a precise answer to the original question: "after x hours, the tx can be changed". Then again, I am skeptical about light wallets implementing this... they would need to keep track of the time they broadcast the tx, assume default node behaviour, and only after timeout, offer an option to bump the fee...? Well I guess the option could always be there, along with some disclaimer (I think this is what we might do in Electrum).

I am in favour of this, hugely because I consider it valuable to have a precise answer (even if it only reflects default behaviour).
I guess @jnewbery meant something else (0-conf being unsecure) when saying "My main argument for this change is that it makes expectations clearer"; still, I feel the same.

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet).

In Electrum, on desktop, all transactions are RBF by default since 3.1 (March 2018);
and low fee transactions have RBF enabled by default since 2.8 (March 2017),
where low fee was defined as feerate < (estimatefee(10)+estimatefee(25))/2
The Android app still prompts the user on every tx whether to make it RBF or not. This is mainly because there are still apps out there that do not display incoming RBF transactions at all until they confirm, which is a horrible experience at a PoS.

@ziggamon
Copy link

ziggamon commented May 9, 2019

@jnewbery

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet). If a user of one of those wallets sends a transaction using the defaults, finds that it's stuck and later wishes to bump the fee, they are unable to.

Right. So we could estimate the usage of these two wallets (Electrum and Bitcoin Core GUI) to what, 10% of payments in a day link, generously? And this error case is a single digit percentage of that, and something that the wallet could prevent. On the other side of the scale we have the 50%+ of payments that are being sent from custodial services or exchanges link. Or the 60% of payments that use something that doesn't even do Segwit yet link. These users will not helped by this, in fact the bitcoin network for them becomes strictly worse. I will elaborate further down.

My main argument for this change is that it makes expectations clearer.

So, Signalpolitik. If that'st he goal then that should be discussed not in a github ticket or IRC but in more broad social angles.

if a transaction is unconfirmed after 6 hours, a malicious actor could easily have double-spent it. Any recipient of a transaction that hasn't been confirmed after that much time should therefore not have any confidence that the tx will ever confirm.

If this were really the case then we wouldn't need this PR, would we? But regardless, this is an assumption that is easily testable. Let's do some tests before we roll out such a big policy change? To my knowledge most tests that are referenced happened in 2012, would be good to have some new data. Happy to let you use Bitrefill to test against, you can buy Lightning channels and just not redeem them and have my word that there will be no trouble. Our findings is that it's reasonably secure for smaller transactions and empirically there hasn't been any systematic abuse of this feature (we've had plenty of attempts to abuse other things)

@SomberNight

which is a horrible experience at a PoS

Earnestly glad that you also value the benefit of still having PoS payments with onchain bitcoin. Not sure what thoughts on this are among the maintainers of this repo. While I too think that all such payments will migrate to Lightning with some time, there is no need to break current onchain policy, Lightning is attractive enough to get people to upgrade without the stick.

By PoS here I mean the classic "scan qr code to send X BTC to Y address" mode that i think we all recognize.
Screenshot 2019-05-09 at 15 09 29

RBF breaks this model for any purchase that is denominated in USD as it allows the user to game the system by waiting indefinitely to see if the Bitcoin prices move. If Bitcoin price goes up while transaction is still unconfirmed - simply cancel it and make a new one for cheaper. If the merchant decides to change the offered price while the payment is in the mempool that opens up for a world of hurt that none of us want.

With RBF this exposure grows beyond the 15 minutes or so that the merchant decides, but it expands to the entire period until the transaction confirms, which in this case can be a very long time (hence the reason for this ticket). This isn't a huge deal when individual wallets have this behavior because you can inform the user about it and have them either disable that feature for next time, or even change wallets to one that will have a better experience. Currently RBF usage is around 7% of transactions link, probably lower of batched payments, so not a huge deal. But if we break the behavior on a policy level it means that services that have a PoS display like the above will need to move to a custodial model (deposit and confirm first, buy stuff later, keep some change on the account). One might make an argument that this mode is better and this is how bitcoin should be working and so on, but that's a bigger discussion than reviewing code in a PR. My experience with bitcoin core is that there is a desire to maintain backwards compatibility as much as possible and not introduce contentious changes.

@jnewbery
Copy link
Contributor

jnewbery commented May 9, 2019

So, Signalpolitik. If that'st he goal then that should be discussed not in a github ticket or IRC but in more broad social angles.

That's not really what I meant. After 6/72/whatever hours, if my node receives a transaction that replaces a tx in my mempool, but with a larger fee, then I can be reasonably sure that the creator of that transaction could get it confirmed ahead of the original transaction. My mempool pretending that's not the case and rejecting the replacement is not helpful to me.

I agree though, this does deserve wider discussion and a github issue isn't really the place for it (and I thank you for taking part in the discussion here!)

Let's do some tests before we roll out such a big policy change? Happy to let you use bitrefill to test against, can buy Lightning channels and just not redeem them.

That sounds fun! It'll be more difficult to double-spend you while blocks aren't consistently full. As you've observed above, this change makes sense in a world where blocks are consistently full. The reason to make this change now is that node policy should be changed in advance of expected changes in the network (see #15846 for example).

Thanks again for your robust input in this PR. It really is very useful to hear from merchants and high-frequency users.

@Sjors
Copy link
Member

Sjors commented May 12, 2019

Bitcoin Core GUI has been using RBF by default for a while. Bitcoin Core RPC doesn't, but I think we should assume RPC users know what they're doing (modulo bugs like #15878). I don't think this change is useful for Bitcoin Core wallet users.

The wallet change does make sense conditional on the node policy change, but because the node only knows it's own policy, it could be source of confusion (especially for non-default settings).

src/validation.cpp Show resolved Hide resolved
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greenaddress Are you still working on this. It would help if all the wallet changes are removed and saved for a later pull request. Generally we wait one release before using new p2p features in the wallet.

@@ -516,6 +516,8 @@ void SetupServerArgs()
gArgs.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (default: %u)", DEFAULT_ENABLE_REPLACEMENT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacementtimeout=<n>", strprintf("Number of seconds after which transactions in mempool can be replaced (default: %u)", DEFAULT_REPLACEMENT_TIMEOUT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-enablewalletreplacementtimeout", strprintf("Whether to enable wallet replacement timeout (default: %u)", DEFAULT_WALLET_REPLACEMENT_TIMEOUT), true, OptionsCategory::OPTIONS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this option entirely and add wallet and rpc support after the p2p change is merged

@laanwj
Copy link
Member

laanwj commented Sep 30, 2019

This PR seems to have gone inactive. Let me know if there's still interest in it and I'll reopen.

@laanwj laanwj closed this Sep 30, 2019
@maflcko
Copy link
Member

maflcko commented Sep 30, 2019

An easy first step would be to make the RPC interface more flexible by reporting a string reason: #16490

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet